Add devfile registry on preferences init when ODO_EXPERIMENTAL=true #3845

johnmcollier · 2020-08-26T17:12:46Z

What type of PR is this?
/kind bug

What does does this PR do / why we need it:
This PR is mostly a backport of johnmcollier@274acb7, which was delivered into the odov2 alpha release

This PR fixes a panic that can be seen when odo registry list is called but the preferences file does not yet exist (and experimental mode is set via ODO_EXPERIMENTAL=true). What was happening was that when retrieve a preferences file, we only add the default devfile registry if the preferences file already exists, if we have to create and initialize the preferences file, we don't add the registry. So a simple solution is to make sure we add the devfile registries when we create and initialize the preferences file.

I also added an integration test case to verify this scenario. As this bug was specific to when ODO_EXPERIMENTAL was set, I had to add some wrapper functions that allow us to pass in env variables to commands in the test framework.

Which issue(s) this PR fixes:

Fixes #3842

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:

Delete ~/.odo/preference.yaml
Run export ODO_EXPERIMENTAL=true
Run odo registry list, verify the default registry is listed and noo panic occurs.

Signed-off-by: John Collier <John.J.Collier@ibm.com>

openshift-ci-robot · 2020-08-26T17:13:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign cdrage
You can assign the PR to them by writing /assign @cdrage in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: John Collier <John.J.Collier@ibm.com>

johnmcollier · 2020-08-27T00:00:12Z

Infra flakes

 2020/08/26 19:49:21 error: unable to signal to artifacts container to terminate in pod integration-e2e-ipi-conf, triggering deletion: could not run remote command: unable to upgrade connection: container not found ("artifacts")
2020/08/26 19:49:21 error: artifacts container for pod integration-e2e-ipi-conf unready: timed out waiting for the condition
2020/08/26 19:49:27 Executing "integration-e2e-gather-aws-console"
2020/08/26 20:19:57 error: unable to signal to artifacts container to terminate in pod integration-e2e-gather-aws-console, triggering deletion: could not run remote command: unable to upgrade connection: container not found ("artifacts")
2020/08/26 20:19:57 error: artifacts container for pod integration-e2e-gather-aws-console unready: timed out waiting for the condition
2020/08/26 20:20:02 Executing "integration-e2e-gather-must-gather"
2020/08/26 20:20:02 The namespace in which this test is executing has been deleted, cancelling the test...

/retest

johnmcollier · 2020-08-28T13:03:27Z

/retest

codecov · 2020-08-28T16:32:12Z

Codecov Report

Merging #3845 into master will decrease coverage by 0.01%.
The diff coverage is 12.50%.

@@            Coverage Diff             @@
##           master    #3845      +/-   ##
==========================================
- Coverage   44.19%   44.17%   -0.02%     
==========================================
  Files         141      141              
  Lines       13600    13608       +8     
==========================================
+ Hits         6010     6011       +1     
- Misses       7007     7013       +6     
- Partials      583      584       +1

Impacted Files	Coverage Δ
pkg/preference/preference.go	`59.59% <12.50%> (-1.99%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68bc98b...c4df0a3. Read the comment docs.

adisky

@johnmcollier Thanks for the fix, please fix here too to avoid panic in future, dereferencing a pointer without checking for nil
https://github.com/openshift/odo/blob/master/pkg/odo/cli/registry/list.go#L60

Signed-off-by: John Collier <John.J.Collier@ibm.com>

johnmcollier · 2020-08-31T16:20:11Z

@adisky Thanks! Good catch. I've updated this PR to include the nil check 👍

cdrage · 2020-08-31T19:04:48Z

pkg/preference/preference.go

+		// Add the default devfile registry if "ODO_EXPERIMENTAL" env variable is true
+		// We specifically need to check for the experimental mode env, as `odo preferences set experimental true` already properly
+		// initializes the default devfile registries.
+		experimentalEnvStr, _ := os.LookupEnv("ODO_EXPERIMENTAL")


Do we need this check? Should we even bother checking if we're in experimental mode? I have applied your patch to my PR here: #3705 which doesn't include looking up experimental and I have all tests passing.

Just out of curiosity.

@cdrage This test https://github.com/openshift/odo/blob/master/tests/integration/component.go#L293 was failing without this check, as devfile components were getting listed without experimental mode being on.

Looks like your branch modifies the test to remove that check (which makes sense, as devfile is being moved out of experimental mode), but that explains why the test isn't failing on your branch

openshift-ci-robot · 2020-08-31T19:41:02Z

@johnmcollier: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/v4.5-integration-e2e	`c4df0a3`	link	`/test v4.5-integration-e2e`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

johnmcollier · 2020-09-01T13:22:55Z

Closing this PR in favour of #3705 which has the same changes

adisky · 2020-09-02T07:30:32Z

@johnmcollier charlie's PR does not have this change https://github.com/openshift/odo/pull/3845/files#diff-0cd3824178637db7a863b63f797b00d4, you could reopen and update this PR OR include this changes in #3705

johnmcollier · 2020-09-02T15:00:10Z

@adisky I'll add a note to #3705 to get the nil check in

johnmcollier added 2 commits August 26, 2020 11:28

Add devfile registry to preferences on init

4307f70

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Add integration test

a37a4c4

Signed-off-by: John Collier <John.J.Collier@ibm.com>

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 26, 2020

openshift-ci-robot requested review from adisky and girishramnani August 26, 2020 17:13

johnmcollier added 2 commits August 26, 2020 13:13

Add integration test

0b290e5

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Add experimental mode gate

9728862

Signed-off-by: John Collier <John.J.Collier@ibm.com>

johnmcollier changed the title ~~Add devfile registry on preferences init~~ Add devfile registry on preferences init when ODO_EXPERIMENTAL=true Aug 26, 2020

adisky suggested changes Aug 31, 2020

View reviewed changes

openshift-ci-robot assigned adisky Aug 31, 2020

Add nil check for registryList pointer

c4df0a3

Signed-off-by: John Collier <John.J.Collier@ibm.com>

cdrage suggested changes Aug 31, 2020

View reviewed changes

openshift-ci-robot assigned cdrage Aug 31, 2020

johnmcollier closed this Sep 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add devfile registry on preferences init when ODO_EXPERIMENTAL=true #3845

Add devfile registry on preferences init when ODO_EXPERIMENTAL=true #3845

johnmcollier commented Aug 26, 2020 •

edited

Loading

openshift-ci-robot commented Aug 26, 2020

johnmcollier commented Aug 27, 2020

johnmcollier commented Aug 28, 2020

codecov bot commented Aug 28, 2020 •

edited

Loading

adisky left a comment

johnmcollier commented Aug 31, 2020

cdrage Aug 31, 2020

johnmcollier Aug 31, 2020

johnmcollier Aug 31, 2020

openshift-ci-robot commented Aug 31, 2020

johnmcollier commented Sep 1, 2020

adisky commented Sep 2, 2020

johnmcollier commented Sep 2, 2020 •

edited

Loading

Add devfile registry on preferences init when ODO_EXPERIMENTAL=true #3845

Add devfile registry on preferences init when ODO_EXPERIMENTAL=true #3845

Conversation

johnmcollier commented Aug 26, 2020 • edited Loading

openshift-ci-robot commented Aug 26, 2020

johnmcollier commented Aug 27, 2020

johnmcollier commented Aug 28, 2020

codecov bot commented Aug 28, 2020 • edited Loading

Codecov Report

adisky left a comment

Choose a reason for hiding this comment

johnmcollier commented Aug 31, 2020

cdrage Aug 31, 2020

Choose a reason for hiding this comment

johnmcollier Aug 31, 2020

Choose a reason for hiding this comment

johnmcollier Aug 31, 2020

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 31, 2020

johnmcollier commented Sep 1, 2020

adisky commented Sep 2, 2020

johnmcollier commented Sep 2, 2020 • edited Loading

johnmcollier commented Aug 26, 2020 •

edited

Loading

codecov bot commented Aug 28, 2020 •

edited

Loading

johnmcollier commented Sep 2, 2020 •

edited

Loading